Skip to content

Conversation

savannahostrowski
Copy link
Member

@savannahostrowski savannahostrowski commented Aug 9, 2025

@savannahostrowski savannahostrowski changed the title GH-132732: Use pure op machinery to optimize various _POP_TOP and _POP_TWO relevant instructions GH-132732: Use pure op machinery to optimize various instructions with _POP_TOP and _POP_TWO Aug 9, 2025
Copy link
Member

@brandtbucher brandtbucher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is super cool! Just a quick run-through for now:

@bedevere-app
Copy link

bedevere-app bot commented Aug 9, 2025

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

Copy link
Member

@Fidget-Spinner Fidget-Spinner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very cool, just one nit.

@savannahostrowski
Copy link
Member Author

@brandtbucher Do you want to take another look at this?

Copy link
Member

@brandtbucher brandtbucher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Just a few suggestions for comments and cleaning up the generated code.

Comment on lines 205 to 211
if (sym_is_const(ctx, res)) {
PyObject *result = sym_get_const(ctx, res);
if (_Py_IsImmortal(result)) {
// Replace with _POP_TOP_LOAD_CONST_INLINE_BORROW since we have one input and an immortal result
REPLACE_OP(this_instr, _POP_TOP_LOAD_CONST_INLINE_BORROW, 0, (uintptr_t)result);
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still seeing weird indent here, maybe dedent the string in the cases generator?

@savannahostrowski savannahostrowski merged commit 9c9a0f7 into python:main Sep 15, 2025
68 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants